Skip to content

Support for TreeBuilder::getRootNode() #126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Feb 11, 2021
Merged

Support for TreeBuilder::getRootNode() #126

merged 4 commits into from
Feb 11, 2021

Conversation

@ruudk
Copy link
Contributor

@ruudk ruudk commented Feb 3, 2021

After seeing #60 stuck I decided to give it a shot...

This is very naive, and it assumes the TreeBuilder is always of type array.

TreeBuilder::__construct(string $name, string $type = 'array', NodeBuilder $builder = null)

If I am able to find the second $type parameter to the new TreeBuilder() call then I can make the return type of getRootNode dynamic based on that.

@ondrejmirtes Is this even possible?

The issue with NodeDefinition::end() is that it can return null when you've reached the root. The current extension removes the nullability in favor of DX. But it's also very naive. No idea if that can be done dynamically.

Hope to get some advice on how to proceed.... 🙏

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Feb 4, 2021

@ruudk Hi, I need a sample of analyzed code and what is this extension trying to achieve.

@ruudk
Copy link
Contributor Author

ruudk commented Feb 4, 2021

@ondrejmirtes I'm trying to let PHPStan understand this: https://github.com/symfony/symfony/blob/60118f14aa33165407b06ed59225a612049f715f/src/Symfony/Bundle/DebugBundle/DependencyInjection/Configuration.php#L30-L55

If you want to reproduce it:

git clone https://github.com/symfony/symfony.git
cd symfony
composer install
composer req --dev phpstan/phpstan:^0.12.71
vendor/bin/phpstan analyse --level=8 src/Symfony/Bundle/DebugBundle/DependencyInjection/Configuration.php

 ------ -----------------------------------------------------------------------
  Line   Configuration.php
 ------ -----------------------------------------------------------------------
  33     Call to an undefined method
         Symfony\Component\Config\Definition\Builder\NodeDefinition::children(
         ).
  58     Call to an undefined method
         Symfony\Component\Config\Definition\Builder\NodeDefinition::children(
         ).
 ------ -----------------------------------------------------------------------


 [ERROR] Found 2 errors

ondrejmirtes
ondrejmirtes requested changes Feb 5, 2021
View reviewed changes
@ruudk ruudk changed the title Add naive extension for TreeBuilder::getRootNode() + NodeDefinition::end() Support for TreeBuilder::getRootNode() Feb 11, 2021
@ruudk
Copy link
Contributor Author

ruudk commented Feb 11, 2021

@ondrejmirtes It's green ✅ 🎉

ondrejmirtes
ondrejmirtes requested changes Feb 11, 2021
View reviewed changes
src/Type/Symfony/TreeBuilderDynamicReturnTypeExtension.php Outdated

$treeBuilder = new $className(
...$args
);
Copy link
Member

@ondrejmirtes ondrejmirtes Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should take a more "static" approach. You can also easily cause an internal error when analyzing code, for example when the constructor call has wrong arguments.

We should have some hardcoded map of what the type => root node types are. And I guess the TreeBuilderType constructor 2nd argument should be ObjectType.

Copy link
Contributor Author

@ruudk ruudk Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the code and I feel it's now much simpler. What do you think?

ruudk added 2 commits February 11, 2021 16:32
@ruudk
Apply feedback
877ed3d
@ruudk
Use class strings
588273d 
I think this is how it's done everywhere.
@ondrejmirtes ondrejmirtes merged commit 6f27071 into phpstan:master Feb 11, 2021
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Feb 11, 2021

Perfect, thank you!

@ondrejmirtes ondrejmirtes mentioned this pull request Feb 11, 2021
Closed
@ruudk ruudk deleted the configuration branch February 11, 2021 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ruudk @ondrejmirtes